cleanup: remove mesh-offline diag logs, plug JWT leaks, throttle retry-loop spam#62
Conversation
…ing to 10m The mesh-offline investigation is resolved. Drop the temporary diagnostic logging added during it, and dial back the telemetry worth keeping. Removed: - OrphanProof MeshID logging in MeshServer_ConnectEx and importSettings (temporary investigation markers). - Two cleartext JWT leaks that printed the full bearer token to logs: the "Openframe token: read JWT" line in token_extractor.c (this reverts that file to its pre-investigation state, superseding #59) and the older "Openframe JWT" line in agentcore.c. Kept the control-channel dial/failure telemetry (fe2ce65): it's throttled, redacts proxy credentials, and carries correlation IDs for future triage. Raised the steady-state throttle from 60s to 10m via a new CONTROL_CHANNEL_LOG_THROTTLE_MS define; target changes still log immediately and suppressed attempts are still counted. Clarified the throttled-log wording so a sampled line reads as one: the dial line is tagged "(latest attempt)" with the counter renamed to suppressed_since_last, and the two throttled "Connection FAILED" lines are tagged "(latest attempt)" too. The un-throttled HTTP-response failure lines are left unmarked since they print on every attempt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… / LOST) Analysis of 80,638 meshcentral-agent lines across 114 client logs showed the control-channel retry loop dominates agent log volume on offline machines: AutoRetry Connect in N ms 22,071 (27% of mesh lines) Control channel disconnected [fd, authState] 12,641 (16%) Connection LOST: ...before full authentication 12,110 (15%) These fire once per ~5-minute reconnect cycle and were un-throttled, making mesh logging ~39% of all client log volume. Gate them on the existing controlChannelLogThisAttempt flag (computed per dial in MeshServer_ConnectEx), so an offline agent's whole per-cycle spam collapses to one logged set per 10 min - the same window as the dial/FAILED lines. Rare or first events still log immediately (a long gap flips the flag back on). The post-auth "Disconnected after authentication" line is left always-on: it's low-volume and marks a real state change (a healthy session closing), not offline spam. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughAdds a centralized throttle constant for control-channel logging, gates dial/retry/disconnect log lines behind a throttle flag, rephrases connection-failure log messages with more structured detail, and removes several debug/diagnostic prints (OrphanProof MeshID, Openframe JWT, decrypted JWT UTC printf) from agentcore.c and token_extractor.c. ChangesControl-channel and token logging reduction
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
meshcore/agentcore.c (1)
4749-4794: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThrottle the DNS-failure retry path too
meshcore/agentcore.c:4626-4631,5009-5011The unresolved-host branch still printsagentcore: Could not resolveon every retry, andMeshServer_Connect()reusescontrolChannelLogThisAttemptfrom the previous attempt. Move the throttle update into the resolve-failure branch or guard that print there so the offline/no-cache path uses the same 10-minute gate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshcore/agentcore.c` around lines 4749 - 4794, The DNS failure retry path still logs on every reconnect because the unresolved-host branch in MeshServer_Connect() bypasses the same throttling used for control-channel attempts. Update the resolve-failure handling near the agentcore “Could not resolve” print to use the same controlChannelLogThisAttempt/controlChannelLastLogTick gating as the dialing path, so repeated offline/no-cache retries only emit once per throttle window. Keep the fix aligned with the existing attempt logging state on agent->controlChannelLogThisAttempt, agent->controlChannelLastLogTick, and agent->controlChannelDialSig.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@meshcore/agentcore.c`:
- Around line 4749-4794: The DNS failure retry path still logs on every
reconnect because the unresolved-host branch in MeshServer_Connect() bypasses
the same throttling used for control-channel attempts. Update the
resolve-failure handling near the agentcore “Could not resolve” print to use the
same controlChannelLogThisAttempt/controlChannelLastLogTick gating as the
dialing path, so repeated offline/no-cache retries only emit once per throttle
window. Keep the fix aligned with the existing attempt logging state on
agent->controlChannelLogThisAttempt, agent->controlChannelLastLogTick, and
agent->controlChannelDialSig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e33c254-5b86-4adf-a9de-41b0e03d2058
📒 Files selected for processing (2)
meshcore/agentcore.copenframe/token_extractor.c
💤 Files with no reviewable changes (1)
- openframe/token_extractor.c
Addresses CodeRabbit review on #62: the "Could not resolve" retry path in MeshServer_ConnectEx bypassed the throttle. When resolution fails (sin6_family == AF_UNSPEC, no proxy, no cached IP), the block that computes controlChannelLogThisAttempt is skipped, so the "agentcore: Could not resolve" print fired on every retry and the stale flag leaked into the downstream AutoRetry log decision. Throttle it here on the same CONTROL_CHANNEL_LOG_THROTTLE_MS (10m) gate, using an "UNRESOLVED" dial signature so a resolved<->unresolved flap still logs immediately, and set controlChannelLogThisAttempt so the AutoRetry log stays consistent. Adds the suppressed_since_last counter for parity with the dial line. Not observed in the current client-log sample (0 occurrences) - offline agents there all resolved DNS and failed at the TCP/HTTP layer - but this closes the one remaining un-throttled retry path for agents offline due to DNS failure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 62 --repo flamingo-stack/meshagent --json commits --jq '.commits[] | "\(.oid) \(.messageHeadline)"' | tail -20
echo "---diff---"
gh api repos/flamingo-stack/meshagent/commits/a93ad18 --jq '.files[].filename'
echo "---patch---"
gh api repos/flamingo-stack/meshagent/commits/a93ad18 --jq '.files[].patch'Length of output: 2277 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd $(fd -t d meshagent -d 1 || echo .) 2>/dev/null || true
fd agentcore.c
FILE=$(fd agentcore.c | head -1)
echo "FILE=$FILE"
grep -n "controlChannelDialSig\|controlChannelLastLogTick\|controlChannelSuppressed\|controlChannelLogThisAttempt\|CONTROL_CHANNEL_LOG_THROTTLE_MS\|ILibGetUptime" -R "$FILE" -n | head -80
echo "---struct definition---"
rg -n "controlChannelDialSig|controlChannelLastLogTick|controlChannelSuppressed|controlChannelLogThisAttempt" --type=c -g '*.h'
</shell>Length of output: 3154 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "controlChannelDialSig|controlChannelLastLogTick|controlChannelSuppressed|controlChannelLogThisAttempt|controlChannelDialTick" git -g '*.h'Length of output: 527 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "controlChannelDialSig|controlChannelLastLogTick|controlChannelSuppressed|controlChannelLogThisAttempt|controlChannelDialTick" --type=h -g '*.h' . || true
fd -e h | xargs grep -ln "controlChannelDialSig" 2>/dev/nullLength of output: 899
No further concerns on this path. Nice cleanup overall across all three commits. [resolve] |
2 participants
Summary
Post-mortem cleanup after the mesh-offline investigation. Removes temporary diagnostic logging, plugs two cleartext JWT leaks, and cuts the control-channel log spam that dominates client log volume.
Two commits:
Why (data)
Analyzed 80,638
tool_id=meshcentral-agentlines across 114 real client logs. Mesh logging was 38.7% of all client log volume, dominated by an offline-retry storm:AutoRetry Connect in N msControl channel disconnectedConnection LOST: ...before full authOpenframe JWT: <token>Connection FAILED: No HTTP responseConnection FAILED: Network timeoutThe JWT leak was logging full bearer tokens across 24 machines in production.
Changes
Removed
OrphanProofMeshID logging (temporary investigation markers) inMeshServer_ConnectExandimportSettings.Openframe token: read JWT ...intoken_extractor.c(reverts that file to pre-investigation state, supersedes fix: make Openframe token diag line parseable by openframe-client #59) and the olderOpenframe JWTline inagentcore.c.Kept, but throttled - the control-channel dial/failure telemetry (redacts proxy creds, carries correlation IDs):
CONTROL_CHANNEL_LOG_THROTTLE_MS.controlChannelLogThisAttemptgate to the per-cycle retry-loop messages (AutoRetry / disconnect / pre-auth LOST), collapsing an offline agent's spam to one logged set per 10m.(latest attempt)marker and renamed the counter tosuppressed_since_last.Deliberately left always-on
Connection FAILED: HTTP <code>lines (they fire only when the server responds non-101 - not offline spam).Connection LOST: Disconnected after authenticationline (rare, meaningful state change).Impact
Estimated ~85-90% reduction in mesh log volume for offline agents, with zero loss of state-transition logs (connect success, handshake, post-auth disconnect). Target changes still log immediately; suppressed attempts are counted.
Testing
Not yet built in this environment (no toolchain configured here). Changes are removals + wording + guarding existing
printfs with an already-computed flag - no format-specifier or arg-count changes.🤖 Generated with Claude Code
Summary by CodeRabbit